Skip to content

Automated Test: oauth-security-enhanced #379

Closed

Conversation

admin-coderabbit
Copy link
Owner

@admin-coderabbit admin-coderabbit commented Feb 4, 2026

This pull request was automatically created by @coderabbitai/e2e-reviewer.

Batch created pull request.

Summary by CodeRabbit

  • New Features

    • Added app credential synchronization system with webhook-based credential sharing support.
    • Implemented centralized credential refresh mechanism across multiple integrations including Google Calendar, Salesforce, Zoom, Webex, and others.
  • Refactor

    • Reorganized OAuth utilities for improved code structure and maintainability.

…11059)

* Add credential sync .env variables

* Add webhook to send app credentials

* Upsert credentials when webhook called

* Refresh oauth token from a specific endpoint

* Pass appSlug

* Add credential encryption

* Move oauth helps into a folder

* Create parse token response wrapper

* Add OAuth helpers to apps

* Clean up

* Refactor `appDirName` to `appSlug`

* Address feedback

* Change to safe parse

* Remove console.log

---------

Co-authored-by: Syed Ali Shahbaz <52925846+alishaz-polymath@users.noreply.github.com>
Co-authored-by: Omar López <zomars@me.com>
@coderabbit-eval
Copy link

coderabbit-eval bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

This pull request reorganizes OAuth utility modules into a dedicated oauth subfolder, introduces credential sharing infrastructure via a new webhook endpoint for synchronizing app credentials, and integrates conditional token refresh utilities across multiple OAuth-based integrations (Google Calendar, Office 365, Zoho, HubSpot, Salesforce, Zoom, Webex, and others).

Changes

Cohort / File(s) Summary
Environment Configuration
.env.example, turbo.json, packages/lib/constants.ts
Added four new environment variables for credential sync webhook (CALCOM_WEBHOOK_SECRET, CALCOM_WEBHOOK_HEADER_NAME, CALCOM_CREDENTIAL_SYNC_ENDPOINT, CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY) and introduced APP_CREDENTIAL_SHARING_ENABLED flag based on presence of encryption key and webhook secret.
Webhook Handler
apps/web/pages/api/webhook/app-credential.ts
New API endpoint for receiving credential sync webhooks; validates requests via secret header, decrypts AES256-encrypted credentials, and creates or updates app credentials in database.
OAuth Token Refresh & Parsing Utilities
packages/app-store/_utils/oauth/refreshOAuthTokens.ts, packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts
New utilities for centralized token refresh (conditionally delegating to external sync endpoint) and token response parsing with schema validation.
OAuth Utilities Refactored (Module Reorganization)
packages/app-store/_utils/oauth/createOAuthAppCredential.ts, packages/app-store/_utils/oauth/decodeOAuthState.ts, packages/app-store/_utils/oauth/encodeOAuthState.ts
OAuth utilities moved from _utils/ root to _utils/oauth/ subfolder; import paths updated within these modules and across all integrations; no functional changes.
Google Calendar Integration
packages/app-store/googlecalendar/api/add.ts, packages/app-store/googlecalendar/api/callback.ts, packages/app-store/googlecalendar/lib/CalendarService.ts
Updated import paths; wrapped token refresh in refreshOAuthTokens and replaced token parsing with parseRefreshTokenResponse.
Office 365 Integrations
packages/app-store/office365calendar/api/..., packages/app-store/office365calendar/lib/CalendarService.ts, packages/app-store/office365video/api/..., packages/app-store/office365video/lib/VideoApiAdapter.ts
Updated import paths and integrated refreshOAuthTokens wrapper and parseRefreshTokenResponse for token handling.
HubSpot Integration
packages/app-store/hubspot/api/add.ts, packages/app-store/hubspot/api/callback.ts, packages/app-store/hubspot/lib/CalendarService.ts
Updated import paths; wrapped token refresh call in refreshOAuthTokens.
Lark Calendar Integration
packages/app-store/larkcalendar/api/add.ts, packages/app-store/larkcalendar/api/callback.ts, packages/app-store/larkcalendar/lib/CalendarService.ts
Updated import paths; replaced fetch-based token refresh with refreshOAuthTokens wrapper.
Salesforce Integration
packages/app-store/salesforce/api/add.ts, packages/app-store/salesforce/api/callback.ts, packages/app-store/salesforce/lib/CalendarService.ts
Updated import paths; added new token refresh flow with zod schema validation and parseRefreshTokenResponse parsing for Salesforce tokens.
Zoom, Webex, Tandem, Stripe, Zoho (CRM & Bigin) Integrations
packages/app-store/zoomvideo/api/..., packages/app-store/zoomvideo/lib/VideoApiAdapter.ts, packages/app-store/webex/api/callback.ts, packages/app-store/webex/lib/VideoApiAdapter.ts, packages/app-store/tandemvideo/api/callback.ts, packages/app-store/stripepayment/api/callback.ts, packages/app-store/zoho*/api/..., packages/app-store/zoho*/lib/...
Updated import paths to oauth subfolder; integrated refreshOAuthTokens and/or parseRefreshTokenResponse into token refresh flows.

Sequence Diagram

sequenceDiagram
    participant External as External System
    participant Webhook as POST /webhook/<br/>app-credential
    participant Validator as Secret &<br/>Schema Validator
    participant Crypto as AES256<br/>Decryption
    participant DB as Database
    
    External->>Webhook: POST with encrypted credentials
    Webhook->>Validator: Check feature flag & webhook secret
    alt Feature disabled or invalid secret
        Validator-->>External: 403 Forbidden
    else Valid
        Validator->>Validator: Validate request body schema
        alt Invalid schema
            Validator-->>External: 400 Bad Request
        else Valid
            Webhook->>Crypto: Decrypt keys with app credential key
            Crypto->>Webhook: Decrypted JSON credentials
            Webhook->>DB: Check user exists
            alt User not found
                DB-->>External: 404 Not Found
            else User exists
                Webhook->>DB: Check app exists by slug
                alt App not found
                    DB-->>External: 404 Not Found
                else App exists
                    Webhook->>DB: Lookup existing credential<br/>by userId + appId
                    alt Credential found
                        DB->>DB: Update credential key
                        DB-->>Webhook: Updated
                        Webhook-->>External: 200 OK (Updated)
                    else Credential not found
                        DB->>DB: Create new credential
                        DB-->>Webhook: Created
                        Webhook-->>External: 200 OK (Created)
                    end
                end
            end
        end
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 A Hop Through Credentials
OAuth tokens now flow through the warren,
Where secrets encrypt what we're borrowin',
Webhooks sync credentials with care,
Centralized refresh—it's only fair!
From Zoom to Salesforce, a unified flair! 🔐✨

🚥 Pre-merge checks | ✅ 1 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Automated Test: oauth-security-enhanced' is generic and does not clearly summarize the main changes. While it references OAuth and security, it fails to convey specific technical details about credential sharing, webhook infrastructure, or the substantial refactoring of OAuth utilities. Consider a more specific title that highlights the main change, such as 'Implement OAuth credential sharing webhook infrastructure' or 'Centralize OAuth token refresh and credential sync logic'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch oauth-security-enhanced

Comment @coderabbit eval help to get the list of available commands and usage tips.

@coderabbit-eval coderabbit-eval bot closed this Feb 4, 2026
Copy link

@coderabbit-eval coderabbit-eval bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 14

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)
packages/app-store/googlecalendar/api/callback.ts (1)

53-80: ⚠️ Potential issue | 🟠 Major

Pre-existing: missing return before res.redirect causes double-redirect.

When state?.installGoogleVideo is true and no existing Google Meet credential is found, res.redirect(...) is called on line 71, but execution falls through to the second res.redirect(...) on line 77. This will attempt to set headers after they've already been sent, resulting in an error at runtime. This is pre-existing and not introduced by this PR, but worth fixing.

Proposed fix
       await prisma.credential.create({
         data: {
           type: "google_video",
           key: {},
           userId: req.session.user.id,
           appId: "google-meet",
         },
       });

-      res.redirect(
+      return res.redirect(
         getSafeRedirectUrl(CAL_URL + "/apps/installed/conferencing?hl=google-meet") ??
           getInstalledAppPath({ variant: "conferencing", slug: "google-meet" })
       );
     }
   }
packages/app-store/salesforce/api/callback.ts (1)

18-21: ⚠️ Potential issue | 🟡 Minor

Pre-existing bug: condition uses && instead of ||, so non-string code values (e.g., arrays) slip through.

code === undefined && typeof code !== "string" is only true when code is undefined (where the second clause is always redundant). If code is an array, both clauses are false so the guard is bypassed. The intent likely was code === undefined || typeof code !== "string".

Not introduced by this PR, but worth fixing while you're here.

Suggested fix
-  if (code === undefined && typeof code !== "string") {
+  if (code === undefined || typeof code !== "string") {
packages/app-store/webex/api/callback.ts (1)

19-27: ⚠️ Potential issue | 🟠 Major

Pre-existing bug: missing = after client_id in the token URL.

Line 20-21 concatenates ...&client_id directly with the client_id value, producing a malformed query parameter (e.g., client_idABC123 instead of client_id=ABC123). This would likely cause the Webex token exchange to fail.

Not introduced by this PR, but this is a correctness issue worth fixing.

Suggested fix
     "https://webexapis.com/v1/access_token?grant_type=authorization_code&client_id" +
-      client_id +
+      "=" + client_id +

Or better, consider using URLSearchParams to construct the query string safely.

packages/app-store/googlecalendar/lib/CalendarService.ts (1)

97-101: ⚠️ Potential issue | 🔴 Critical

Bug: storing the SafeParseReturnType object instead of parsed credentials.

parseRefreshTokenResponse returns a safeParse-like result ({ success, data, error }), as seen in Zoom (line 104, checks .success) and Office365 usage. Here, the entire result object is assigned to key and written to the DB. Previously, googleCredentialSchema.parse(...) returned the parsed data directly.

This will persist { success: true, data: { access_token, ... } } instead of { access_token, ... }, corrupting the stored credential and causing token retrieval to fail on subsequent reads (line 75 parses credential.key with googleCredentialSchema).

🐛 Proposed fix
-      const key = parseRefreshTokenResponse(googleCredentials, googleCredentialSchema);
+      const parsedKey = parseRefreshTokenResponse(googleCredentials, googleCredentialSchema);
+      if (!parsedKey.success) {
+        this.log.error("Invalid refreshed tokens were returned for Google Calendar");
+        return myGoogleAuth;
+      }
+      const key = parsedKey.data;
       await prisma.credential.update({
         where: { id: credential.id },
         data: { key },
       });
packages/app-store/salesforce/lib/CalendarService.ts (1)

96-108: ⚠️ Potential issue | 🔴 Critical

Bug: prisma is not imported and refreshed tokens are not used for the connection.

Two issues:

  1. prisma is used on line 96 but never imported in this file. This will fail at compile/runtime with a ReferenceError.

  2. The jsforce.Connection on lines 101-108 still uses the old credentialKey values (credentialKey.instance_url, credentialKey.access_token) instead of the freshly refreshed accessTokenParsed.data. The refreshed tokens are saved to the DB but the current request will use the expired token, likely causing API failures.

🐛 Proposed fix

Add the missing import at the top of the file:

import prisma from "@calcom/prisma";

Then use the refreshed credentials for the connection:

     await prisma.credential.update({
       where: { id: credential.id },
       data: { key: { ...accessTokenParsed.data, refresh_token: credentialKey.refresh_token } },
     });

     return new jsforce.Connection({
       clientId: consumer_key,
       clientSecret: consumer_secret,
       redirectUri: WEBAPP_URL + "/api/integrations/salesforce/callback",
-      instanceUrl: credentialKey.instance_url,
-      accessToken: credentialKey.access_token,
+      instanceUrl: accessTokenParsed.data.instance_url,
+      accessToken: accessTokenParsed.data.access_token,
       refreshToken: credentialKey.refresh_token,
     });
🤖 Fix all issues with AI agents
In `@apps/web/pages/api/webhook/app-credential.ts`:
- Line 31: Replace the unconditional call to
appCredentialWebhookRequestBodySchema.parse with a safe parse flow: call
appCredentialWebhookRequestBodySchema.safeParse(req.body), check the
result.success flag, and if false respond with a 400 (Bad Request) including the
validation errors; otherwise assign the parsed data to reqBody and continue.
Update references to reqBody and preserve existing handler logic so only invalid
bodies return early with a 400 and validation details.
- Around line 57-59: The decryption and JSON parsing of reqBody.keys using
symmetricDecrypt(...) and JSON.parse(...) can throw and are currently unhandled;
wrap the call that assigns keys in a try-catch around
symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY
|| "") and JSON.parse(...) (the code that assigns the keys variable), catch
errors, log a non-sensitive error message, and return a safe client error
response (e.g., 400 with a generic "invalid credentials" message) instead of
letting the exception propagate; ensure no sensitive details or stack traces are
returned to the client.
- Around line 17-29: The handler must reject non-POSTs and must not allow a
missing secret to bypass validation: in the exported handler function, first
enforce req.method === "POST" and return 405 for others; then ensure
process.env.CALCOM_WEBHOOK_SECRET is defined (if not, return 403) before
comparing it to the incoming header (use process.env.CALCOM_WEBHOOK_HEADER_NAME
|| "calcom-webhook-secret" to read the header); finally perform the strict
compare between the known secret and the header value and return 403 on
mismatch. Reference: handler, APP_CREDENTIAL_SHARING_ENABLED,
CALCOM_WEBHOOK_SECRET, CALCOM_WEBHOOK_HEADER_NAME.

In `@packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts`:
- Around line 25-27: The code sets refreshTokenResponse.data.refresh_token to
the literal "refresh_token", which masks the absence of a real token; instead,
stop injecting that sentinel: leave refreshTokenResponse.data.refresh_token as
undefined or null when the provider omits it so downstream logic can detect "no
refresh token". Update the conditional in parseRefreshTokenResponse to remove
the assignment and ensure downstream consumers (any code reading
refreshTokenResponse.data.refresh_token) handle undefined/null distinctly from a
real token.
- Around line 5-11: minimumTokenResponseSchema is incorrectly using computed-key
syntax which creates literal strange keys and also drops unknown properties;
replace the object schema with a schema that accepts access_token plus arbitrary
provider fields by defining z.object({ access_token: z.string() }).passthrough()
(or, if you need to enforce numeric expiry values, validate those separately
with z.record(z.number()) or a refinement/transform), and remove the
computed-key entries so expiry/refresh/provider-specific fields are preserved by
parseRefreshTokenResponse.

In `@packages/app-store/_utils/oauth/refreshOAuthTokens.ts`:
- Around line 3-20: refreshOAuthTokens currently returns a raw fetch Response
when APP_CREDENTIAL_SHARING_ENABLED is true but returns the provider-specific
value from refreshFunction() otherwise, causing downstream callers (expecting
AxiosResponse, HubSpot token object, etc.) to break; update refreshOAuthTokens
to always return a normalized contract: call
fetch(process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT, ...) then await
response.json(), validate/transform that JSON into the same shape(s) your
callers expect (or into a simple common wrapper { provider: string, data: any,
expiryDate?: string, error?: any }), and return that normalized object so code
using refreshFunction(), refreshOAuthTokens, and callers (which inspect
tokenInfo.data, .expiryDate, or .data.error) receive a consistent structure;
keep the refreshFunction() fallback behavior but adapt/normalize its result to
the same contract as well.
- Around line 8-15: Wrap the fetch to
process.env.CALCOM_CREDENTIAL_SYNC_ENDPOINT in a try/catch, check response.ok
after the await fetch, and throw a descriptive error (including response.status
and response.statusText or response body) when not ok; specifically update the
code around the fetch call (the use of fetch, URLSearchParams, and the returned
response) so network errors are caught and a clear error is thrown on non-2xx
responses before returning the successful response or parsed body.

In `@packages/app-store/googlecalendar/lib/CalendarService.ts`:
- Around line 86-93: The Google token refresh call in CalendarService is
assuming a GaxiosResponse shape (res?.data) but refreshOAuthTokens can return a
standard fetch Response when APP_CREDENTIAL_SHARING_ENABLED, so update the
handling around the result of refreshOAuthTokens (the call wrapping
myGoogleAuth.refreshToken and the subsequent use of token.access_token) to
detect and normalize both shapes: if res has a .data property use that,
otherwise parse the fetch Response body (use the existing helper
handleErrorsJson or similar) to extract the JSON and assign it to token before
accessing token.access_token; adjust the code paths around refreshOAuthTokens,
myGoogleAuth.refreshToken, googleCredentials and token to ensure a unified token
object is used.

In `@packages/app-store/hubspot/lib/CalendarService.ts`:
- Around line 177-189: refreshOAuthTokens can return either a Token-like object
or a raw fetch Response when credential sharing is enabled, but
CalendarService.ts immediately reads hubspotRefreshToken.expiresIn (and other
Token fields), causing NaN/undefined behavior; update the code around the call
to refreshOAuthTokens (the invocation that passes
hubspotClient.oauth.tokensApi.createToken) to detect if the result is a fetch
Response and, if so, await response.json() and map/normalize its fields to the
HubspotToken shape (accessToken, expiresIn, refreshToken, etc.) before using
expiresIn to compute expiryDate and before updating credentials; alternatively
add a type guard/assertion to ensure hubspotRefreshToken matches the expected
Token shape and throw/log a clear error if not.

In `@packages/app-store/salesforce/lib/CalendarService.ts`:
- Line 86: In CalendarService, the current check uses response.statusText which
is unreliable; change the condition to use response.ok (or check
response.status) when validating the fetch result: replace the `if
(response.statusText !== "OK") throw new HttpError({ statusCode: 400, message:
response.statusText });` with a check like `if (!response.ok) throw new
HttpError({ statusCode: response.status, message: response.statusText ||
String(response.status) });` so the code throws with the real HTTP status and a
fallback message; update the `response` usage and HttpError construction
accordingly.

In `@packages/app-store/zoho-bigin/lib/CalendarService.ts`:
- Around line 85-94: The call to refreshOAuthTokens is passing credentialId (the
DB PK) but the function expects the user's ID (calcomUserId) — use
credential.userId instead. Modify the code that defines credentialId (and/or the
refreshAccessToken / biginAuth flow) to also carry credential.userId and update
the call to refreshOAuthTokens(..., "zoho-bigin", credential.userId); if
refreshAccessToken or biginAuth currently only accept credentialId, add a
parameter (or attach userId to the credential object) so you can pass
credential.userId through to refreshOAuthTokens.

In `@packages/app-store/zohocrm/lib/CalendarService.ts`:
- Line 219: The token expiry calculation sets zohoCrmTokenInfo.data.expiryDate
incorrectly by adding 60*60 (milliseconds) to Date.now(), producing an expiry
~3.6 seconds away; update the assignment in CalendarService where
zohoCrmTokenInfo.data.expiryDate is set to add milliseconds for one hour
(multiply by 1000) — e.g., replace the current expression with Date.now() + 60 *
60 * 1000 (optionally wrapped in Math.round or Math.floor) so the expiry is one
hour in the future.

In `@packages/app-store/zoomvideo/lib/VideoApiAdapter.ts`:
- Line 104: The call to parseRefreshTokenResponse(…, zoomRefreshedTokenSchema)
in VideoApiAdapter.ts now throws on failure, so the existing if
(!parsedToken.success) block is dead; wrap the parseRefreshTokenResponse call in
a try-catch instead: call parseRefreshTokenResponse and assign to parsedToken
inside try, and in catch log the error (include context like "failed to parse
refresh token") and rethrow or convert to the adapter's expected error/return
form so callers get a clear failure; update any references to parsedToken
afterward to assume a successful parse when no exception was thrown.

In `@packages/lib/constants.ts`:
- Around line 103-104: APP_CREDENTIAL_SHARING_ENABLED currently evaluates to the
raw CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY string when both env vars exist; coerce
the expression to a boolean so the key value is never exposed. Change the
initializer for APP_CREDENTIAL_SHARING_ENABLED to return a strict boolean based
on the presence of CALCOM_WEBHOOK_SECRET and
CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY (e.g., wrap the combined check with
Boolean(...) or use double-negation) so imports of
APP_CREDENTIAL_SHARING_ENABLED cannot leak secret values.
🧹 Nitpick comments (6)
packages/app-store/zoho-bigin/api/add.ts (1)

17-17: Hard-coded slug replaces dynamic appConfig.slug in redirect URI.

The redirect URI now uses a hard-coded "zoho-bigin" instead of interpolating appConfig.slug. Note that appConfig.slug is still used on line 12 for getAppKeysFromSlug. If the slug in config.json ever drifts from "zoho-bigin", this would create a mismatch between the key lookup and the redirect URI.

Consider using appConfig.slug consistently:

Suggested change
-    const redirectUri = WEBAPP_URL + `/api/integrations/zoho-bigin/callback`;
+    const redirectUri = WEBAPP_URL + `/api/integrations/${appConfig.slug}/callback`;
turbo.json (1)

205-207: CALCOM_WEBHOOK_SECRET is out of alphabetical order.

It's placed after CALENDSO_ENCRYPTION_KEY, but CALCOM_W... sorts before CALEND.... Swap lines 207 and 206 to maintain the alphabetical convention used throughout this array.

Proposed fix
     "CALCOM_WEBHOOK_HEADER_NAME",
-    "CALENDSO_ENCRYPTION_KEY",
     "CALCOM_WEBHOOK_SECRET",
+    "CALENDSO_ENCRYPTION_KEY",
     "CI",
packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts (1)

13-30: safeParse result type is not narrowed — refreshTokenResponse.data may be typed as unknown.

When using schema.safeParse() where schema is z.ZodTypeAny, the inferred data type is any. This is acceptable at runtime but loses type safety. Additionally, the function mutates .data.refresh_token on the safe-parse result object — modifying the parse result in-place is fragile. Consider constructing a new object with the defaults spread in.

packages/app-store/webex/lib/VideoApiAdapter.ts (1)

151-151: Debug console.log statements should be removed before merging.

Lines 151, 184–186, 194, and 223 contain console.log calls that appear to be debug artifacts (e.g., "result of accessToken in fetchWebexApi", "meting body"). These leak implementation details at runtime and should be removed or converted to structured logger calls.

packages/app-store/office365calendar/lib/CalendarService.ts (1)

263-264: Silent failure when token parsing fails.

If parseRefreshTokenResponse returns { success: false }, the spread on line 264 (...(false)) is a no-op, so stale credentials are silently re-saved to the DB (lines 265-272) without any log or error signal. The AI summary notes that inline error logging for token parsing failure was removed here. Consider adding a warning log to aid debugging when token refresh silently fails.

💡 Proposed improvement
     const tokenResponse = parseRefreshTokenResponse(responseJson, refreshTokenResponseSchema);
+    if (!tokenResponse.success) {
+      this.log.warn("Office365 token refresh: failed to parse refreshed token response");
+    }
     o365AuthCredentials = { ...o365AuthCredentials, ...(tokenResponse.success && tokenResponse.data) };
apps/web/pages/api/webhook/app-credential.ts (1)

16-16: Empty JSDoc comment.

The /** */ comment on line 16 appears to be an incomplete placeholder. Either add meaningful documentation for the handler or remove it.

Comment on lines +17 to +29
export default async function handler(req: NextApiRequest, res: NextApiResponse) {
// Check that credential sharing is enabled
if (!APP_CREDENTIAL_SHARING_ENABLED) {
return res.status(403).json({ message: "Credential sharing is not enabled" });
}

// Check that the webhook secret matches
if (
req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !==
process.env.CALCOM_WEBHOOK_SECRET
) {
return res.status(403).json({ message: "Invalid webhook secret" });
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing HTTP method restriction and brittle webhook secret validation.

  1. No method check: This handler accepts any HTTP method. It should restrict to POST only.

  2. Webhook secret bypass when unset: If CALCOM_WEBHOOK_SECRET is undefined and the request omits the header, both sides are undefined, so undefined !== undefined evaluates to false and the check passes. The APP_CREDENTIAL_SHARING_ENABLED guard on line 19 mitigates this (since it requires the env var to be set), but defense-in-depth is warranted.

🛡️ Proposed fix
 export default async function handler(req: NextApiRequest, res: NextApiResponse) {
+  if (req.method !== "POST") {
+    return res.status(405).json({ message: "Method not allowed" });
+  }
+
   // Check that credential sharing is enabled
   if (!APP_CREDENTIAL_SHARING_ENABLED) {
     return res.status(403).json({ message: "Credential sharing is not enabled" });
   }

   // Check that the webhook secret matches
+  const webhookSecret = process.env.CALCOM_WEBHOOK_SECRET;
+  if (!webhookSecret) {
+    return res.status(500).json({ message: "Webhook secret is not configured" });
+  }
   if (
     req.headers[process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret"] !==
-    process.env.CALCOM_WEBHOOK_SECRET
+    webhookSecret
   ) {
     return res.status(403).json({ message: "Invalid webhook secret" });
   }
🤖 Prompt for AI Agents
In `@apps/web/pages/api/webhook/app-credential.ts` around lines 17 - 29, The
handler must reject non-POSTs and must not allow a missing secret to bypass
validation: in the exported handler function, first enforce req.method ===
"POST" and return 405 for others; then ensure process.env.CALCOM_WEBHOOK_SECRET
is defined (if not, return 403) before comparing it to the incoming header (use
process.env.CALCOM_WEBHOOK_HEADER_NAME || "calcom-webhook-secret" to read the
header); finally perform the strict compare between the known secret and the
header value and return 403 on mismatch. Reference: handler,
APP_CREDENTIAL_SHARING_ENABLED, CALCOM_WEBHOOK_SECRET,
CALCOM_WEBHOOK_HEADER_NAME.

return res.status(403).json({ message: "Invalid webhook secret" });
}

const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Unhandled ZodError on invalid request body.

z.parse() throws a ZodError if the body doesn't match the schema, resulting in an unhandled 500 with a stack trace. Use .safeParse() and return a 400 for invalid input.

🛡️ Proposed fix
-  const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body);
+  const parsed = appCredentialWebhookRequestBodySchema.safeParse(req.body);
+  if (!parsed.success) {
+    return res.status(400).json({ message: "Invalid request body" });
+  }
+  const reqBody = parsed.data;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const reqBody = appCredentialWebhookRequestBodySchema.parse(req.body);
const parsed = appCredentialWebhookRequestBodySchema.safeParse(req.body);
if (!parsed.success) {
return res.status(400).json({ message: "Invalid request body" });
}
const reqBody = parsed.data;
🤖 Prompt for AI Agents
In `@apps/web/pages/api/webhook/app-credential.ts` at line 31, Replace the
unconditional call to appCredentialWebhookRequestBodySchema.parse with a safe
parse flow: call appCredentialWebhookRequestBodySchema.safeParse(req.body),
check the result.success flag, and if false respond with a 400 (Bad Request)
including the validation errors; otherwise assign the parsed data to reqBody and
continue. Update references to reqBody and preserve existing handler logic so
only invalid bodies return early with a 400 and validation details.

Comment on lines +57 to +59
const keys = JSON.parse(
symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Unhandled errors from decryption and JSON parsing.

Both symmetricDecrypt (on invalid ciphertext) and JSON.parse (on malformed JSON) can throw. Without a try-catch, these surface as 500 errors with potentially sensitive stack traces.

🛡️ Proposed fix
-  const keys = JSON.parse(
-    symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
-  );
+  let keys;
+  try {
+    keys = JSON.parse(
+      symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
+    );
+  } catch (error) {
+    return res.status(400).json({ message: "Failed to decrypt or parse credential keys" });
+  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const keys = JSON.parse(
symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
);
let keys;
try {
keys = JSON.parse(
symmetricDecrypt(reqBody.keys, process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "")
);
} catch (error) {
return res.status(400).json({ message: "Failed to decrypt or parse credential keys" });
}
🤖 Prompt for AI Agents
In `@apps/web/pages/api/webhook/app-credential.ts` around lines 57 - 59, The
decryption and JSON parsing of reqBody.keys using symmetricDecrypt(...) and
JSON.parse(...) can throw and are currently unhandled; wrap the call that
assigns keys in a try-catch around symmetricDecrypt(reqBody.keys,
process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY || "") and JSON.parse(...) (the
code that assigns the keys variable), catch errors, log a non-sensitive error
message, and return a safe client error response (e.g., 400 with a generic
"invalid credentials" message) instead of letting the exception propagate;
ensure no sensitive details or stack traces are returned to the client.

Comment on lines +5 to +11
const minimumTokenResponseSchema = z.object({
access_token: z.string(),
// Assume that any property with a number is the expiry
[z.string().toString()]: z.number(),
// Allow other properties in the token response
[z.string().optional().toString()]: z.unknown().optional(),
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, verify the file exists and check its content
find . -name "parseRefreshTokenResponse.ts" -type f

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 152


🏁 Script executed:

#!/bin/bash
# Test the Zod schema behavior directly
cd /tmp
cat > test_schema.mjs << 'EOF'
import { z } from "zod";

console.log("=== Testing z.string().toString() ===");
console.log("z.string().toString() =>", z.string().toString());
console.log("z.string().optional().toString() =>", z.string().optional().toString());

console.log("\n=== Testing current schema ===");
const currentSchema = z.object({
  access_token: z.string(),
  [z.string().toString()]: z.number(),
  [z.string().optional().toString()]: z.unknown().optional(),
});

const testResponse = {
  access_token: "test_token",
  refresh_token: "test_refresh",
  expires_in: 3600,
  expiry_date: 1700000000,
};

const result = currentSchema.safeParse(testResponse);
console.log("Parse success:", result.success);
console.log("Input keys:", Object.keys(testResponse));
console.log("Output keys:", result.success ? Object.keys(result.data) : "N/A");
console.log("Full output:", JSON.stringify(result.data || result.error.issues, null, 2));

console.log("\n=== Testing with passthrough ===");
const passthroughSchema = z.object({
  access_token: z.string(),
}).passthrough();

const result2 = passthroughSchema.safeParse(testResponse);
console.log("Parse success:", result2.success);
console.log("Output keys:", result2.success ? Object.keys(result2.data) : "N/A");
console.log("Full output:", JSON.stringify(result2.data, null, 2));
EOF

# Try to run with node
node test_schema.mjs 2>&1 || echo "Node execution failed or not available"

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1039


🏁 Script executed:

#!/bin/bash
# Read the file to verify the schema definition
cat -n ./packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1344


minimumTokenResponseSchema does not work as intended — computed-key syntax is not a Zod catch-all.

[z.string().toString()] evaluates to a literal string key (the toString() representation of the Zod schema object), not a dynamic record key matcher. The schema ends up defining two oddly-named literal keys rather than "any string key → number" and "any optional key → unknown". Additionally, z.object() strips unrecognized keys by default, so expiry fields, refresh tokens, and other provider-specific properties will be silently dropped from the parsed output.

To accept access_token plus arbitrary additional fields, use .passthrough():

Proposed fix
-const minimumTokenResponseSchema = z.object({
-  access_token: z.string(),
-  //   Assume that any property with a number is the expiry
-  [z.string().toString()]: z.number(),
-  //   Allow other properties in the token response
-  [z.string().optional().toString()]: z.unknown().optional(),
-});
+const minimumTokenResponseSchema = z
+  .object({
+    access_token: z.string(),
+  })
+  .passthrough();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const minimumTokenResponseSchema = z.object({
access_token: z.string(),
// Assume that any property with a number is the expiry
[z.string().toString()]: z.number(),
// Allow other properties in the token response
[z.string().optional().toString()]: z.unknown().optional(),
});
const minimumTokenResponseSchema = z
.object({
access_token: z.string(),
})
.passthrough();
🤖 Prompt for AI Agents
In `@packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts` around lines 5
- 11, minimumTokenResponseSchema is incorrectly using computed-key syntax which
creates literal strange keys and also drops unknown properties; replace the
object schema with a schema that accepts access_token plus arbitrary provider
fields by defining z.object({ access_token: z.string() }).passthrough() (or, if
you need to enforce numeric expiry values, validate those separately with
z.record(z.number()) or a refinement/transform), and remove the computed-key
entries so expiry/refresh/provider-specific fields are preserved by
parseRefreshTokenResponse.

Comment on lines +25 to +27
if (!refreshTokenResponse.data.refresh_token) {
refreshTokenResponse.data.refresh_token = "refresh_token";
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Setting refresh_token to the literal string "refresh_token" is a risky sentinel value.

If a provider legitimately omits refresh_token (e.g., because it doesn't rotate refresh tokens), injecting a dummy string will cause a hard failure on the next token refresh attempt — but with a confusing error from the provider rather than a clear local error. Consider leaving it undefined or null so downstream code can distinguish "no refresh token" from "has a refresh token."

🤖 Prompt for AI Agents
In `@packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts` around lines 25
- 27, The code sets refreshTokenResponse.data.refresh_token to the literal
"refresh_token", which masks the absence of a real token; instead, stop
injecting that sentinel: leave refreshTokenResponse.data.refresh_token as
undefined or null when the provider omits it so downstream logic can detect "no
refresh token". Update the conditional in parseRefreshTokenResponse to remove
the assignment and ensure downstream consumers (any code reading
refreshTokenResponse.data.refresh_token) handle undefined/null distinctly from a
real token.

}),
});

if (response.statusText !== "OK") throw new HttpError({ statusCode: 400, message: response.statusText });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fragile status check: response.statusText is unreliable.

response.statusText is not guaranteed to be "OK" even for 200 responses — the Fetch spec allows servers to send any status text, and HTTP/2 doesn't include it at all. Use response.ok instead.

🐛 Proposed fix
-    if (response.statusText !== "OK") throw new HttpError({ statusCode: 400, message: response.statusText });
+    if (!response.ok) throw new HttpError({ statusCode: response.status, message: response.statusText });
🤖 Prompt for AI Agents
In `@packages/app-store/salesforce/lib/CalendarService.ts` at line 86, In
CalendarService, the current check uses response.statusText which is unreliable;
change the condition to use response.ok (or check response.status) when
validating the fetch result: replace the `if (response.statusText !== "OK")
throw new HttpError({ statusCode: 400, message: response.statusText });` with a
check like `if (!response.ok) throw new HttpError({ statusCode: response.status,
message: response.statusText || String(response.status) });` so the code throws
with the real HTTP status and a fallback message; update the `response` usage
and HttpError construction accordingly.

Comment on lines +85 to +94
const tokenInfo = await refreshOAuthTokens(
async () =>
await axios.post(accountsUrl, qs.stringify(formData), {
headers: {
"Content-Type": "application/x-www-form-urlencoded;charset=utf-8",
},
}),
"zoho-bigin",
credentialId
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Wrong identifier passed as userIdcredentialId is the credential's PK, not the user's ID.

credentialId (from line 53: const credentialId = credential.id) is the credential row's database primary key, not the user ID. The refreshOAuthTokens function expects userId (used as calcomUserId in the sync endpoint payload). This will send an incorrect identifier to the credential sync endpoint when sharing is enabled.

All other callers in this PR pass credential.userId. This file should do the same.

Proposed fix

The refreshAccessToken method needs access to credential.userId. Either store credential (or credential.userId) alongside credentialId, or pass it through:

-  private async refreshAccessToken(credentialId: number, credentialKey: BiginToken) {
+  private async refreshAccessToken(credential: CredentialPayload, credentialKey: BiginToken) {
+    const credentialId = credential.id;
     ...
     const tokenInfo = await refreshOAuthTokens(
       async () =>
         await axios.post(accountsUrl, qs.stringify(formData), {
           headers: {
             "Content-Type": "application/x-www-form-urlencoded;charset=utf-8",
           },
         }),
       "zoho-bigin",
-      credentialId
+      credential.userId
     );

And update the call site in biginAuth accordingly.

🤖 Prompt for AI Agents
In `@packages/app-store/zoho-bigin/lib/CalendarService.ts` around lines 85 - 94,
The call to refreshOAuthTokens is passing credentialId (the DB PK) but the
function expects the user's ID (calcomUserId) — use credential.userId instead.
Modify the code that defines credentialId (and/or the refreshAccessToken /
biginAuth flow) to also carry credential.userId and update the call to
refreshOAuthTokens(..., "zoho-bigin", credential.userId); if refreshAccessToken
or biginAuth currently only accept credentialId, add a parameter (or attach
userId to the credential object) so you can pass credential.userId through to
refreshOAuthTokens.

}),
"zohocrm",
credential.userId
);
if (!zohoCrmTokenInfo.data.error) {
// set expiry date as offset from current time.
zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + 60 * 60);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Pre-existing bug: expiry offset is ~3.6 seconds, not 1 hour.

Math.round(Date.now() + 60 * 60) adds 3600 milliseconds to Date.now() (which is in ms). This sets the token expiry to ~3.6 seconds from now, causing near-immediate re-refresh on every call. Should likely be Date.now() + 60 * 60 * 1000.

This is pre-existing and not introduced by this PR, but worth flagging.

Proposed fix
-          zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + 60 * 60);
+          zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + 60 * 60 * 1000);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + 60 * 60);
zohoCrmTokenInfo.data.expiryDate = Math.round(Date.now() + 60 * 60 * 1000);
🤖 Prompt for AI Agents
In `@packages/app-store/zohocrm/lib/CalendarService.ts` at line 219, The token
expiry calculation sets zohoCrmTokenInfo.data.expiryDate incorrectly by adding
60*60 (milliseconds) to Date.now(), producing an expiry ~3.6 seconds away;
update the assignment in CalendarService where zohoCrmTokenInfo.data.expiryDate
is set to add milliseconds for one hour (multiply by 1000) — e.g., replace the
current expression with Date.now() + 60 * 60 * 1000 (optionally wrapped in
Math.round or Math.floor) so the expiry is one hour in the future.

@@ -94,7 +101,7 @@ const zoomAuth = (credential: CredentialPayload) => {
}
}
// We check the if the new credentials matches the expected response structure
const parsedToken = zoomRefreshedTokenSchema.safeParse(responseBody);
const parsedToken = parseRefreshTokenResponse(responseBody, zoomRefreshedTokenSchema);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

fd parseRefreshTokenResponse --type f

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 150


🏁 Script executed:

cat -n packages/app-store/zoomvideo/lib/VideoApiAdapter.ts | head -150 | tail -50

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 2338


🏁 Script executed:

cat -n packages/app-store/_utils/oauth/parseRefreshTokenResponse.ts

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1344


Behavioral difference confirmed: parseRefreshTokenResponse throws instead of returning falsy result.

The check on line 108 (if (!parsedToken.success)) is now dead code. parseRefreshTokenResponse throws an error on parse failure (line 21-22 in parseRefreshTokenResponse.ts) rather than returning { success: false, ... }, so the success flag will always be true if execution reaches line 111. The error message on line 109 is unreachable.

Update the code to either:

  • Remove the dead check (lines 108-110) since errors are thrown, or
  • Wrap the call in try-catch to handle the thrown error
🤖 Prompt for AI Agents
In `@packages/app-store/zoomvideo/lib/VideoApiAdapter.ts` at line 104, The call to
parseRefreshTokenResponse(…, zoomRefreshedTokenSchema) in VideoApiAdapter.ts now
throws on failure, so the existing if (!parsedToken.success) block is dead; wrap
the parseRefreshTokenResponse call in a try-catch instead: call
parseRefreshTokenResponse and assign to parsedToken inside try, and in catch log
the error (include context like "failed to parse refresh token") and rethrow or
convert to the adapter's expected error/return form so callers get a clear
failure; update any references to parsedToken afterward to assume a successful
parse when no exception was thrown.

Comment on lines +103 to +104
export const APP_CREDENTIAL_SHARING_ENABLED =
process.env.CALCOM_WEBHOOK_SECRET && process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

APP_CREDENTIAL_SHARING_ENABLED leaks the encryption key value — coerce to boolean.

Because && returns the second operand's value when both are truthy, this constant holds the raw CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY string. Any code importing APP_CREDENTIAL_SHARING_ENABLED (e.g., for logging or error context) could inadvertently expose the encryption key.

🔒 Proposed fix
 export const APP_CREDENTIAL_SHARING_ENABLED =
-  process.env.CALCOM_WEBHOOK_SECRET && process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY;
+  !!(process.env.CALCOM_WEBHOOK_SECRET && process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const APP_CREDENTIAL_SHARING_ENABLED =
process.env.CALCOM_WEBHOOK_SECRET && process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY;
export const APP_CREDENTIAL_SHARING_ENABLED =
!!(process.env.CALCOM_WEBHOOK_SECRET && process.env.CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY);
🤖 Prompt for AI Agents
In `@packages/lib/constants.ts` around lines 103 - 104,
APP_CREDENTIAL_SHARING_ENABLED currently evaluates to the raw
CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY string when both env vars exist; coerce the
expression to a boolean so the key value is never exposed. Change the
initializer for APP_CREDENTIAL_SHARING_ENABLED to return a strict boolean based
on the presence of CALCOM_WEBHOOK_SECRET and
CALCOM_APP_CREDENTIAL_ENCRYPTION_KEY (e.g., wrap the combined check with
Boolean(...) or use double-negation) so imports of
APP_CREDENTIAL_SHARING_ENABLED cannot leak secret values.

Sign in to join this conversation on GitHub.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant